Skip to content

perf(desktop): reduce UI stalls during tool output - #3921

Open
Colafornia wants to merge 1 commit into
apache:mainfrom
Colafornia:fix/desktop-tool-output-jank
Open

perf(desktop): reduce UI stalls during tool output#3921
Colafornia wants to merge 1 commit into
apache:mainfrom
Colafornia:fix/desktop-tool-output-jank

Conversation

@Colafornia

@Colafornia Colafornia commented Aug 26, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Summary

Live tool output currently publishes every chunk directly to Renderer state. Frequent chunks trigger repeated React updates within one display frame.

This can make the interface feel unresponsive while the model is working. Expanding tool details can lag, and loading animations can drop frames.

  • Before: each tool_output_delta publishes a separate state update.
  • After: deltas received within one animation frame publish as one update.

All output chunks remain ordered and complete. Lifecycle and readiness events remain synchronous and flush pending output before they run. Session teardown discards its queued events so a delayed frame cannot restore cleared state.

No screenshot or recording is included. This problem affects transient frame pacing during live streaming, which static images cannot show reliably.

Verification

  • npm --workspace @maka/desktop run build:main
  • node --test apps/desktop/dist/main/__tests__/streaming-handoff.test.js
  • Scoped Biome check passed for the three affected files.
  • git diff --check
  • Regression tests cover frame batching and delayed frames after session teardown.
  • Manual Desktop testing showed better responsiveness while the model and tools were active.

AI use

Select exactly one:

  • No generative tool made a substantive contribution
  • Generative tooling made a substantive contribution

Tool(s) and scope: Codex traced the Renderer event flow, implemented frame batching and the session teardown fix, added regression tests, and ran the listed checks. The affected commits include Generated-by: Codex trailers.

Checklist

  • Tests cover the change and fail without it
  • Lint, format, typecheck and the affected suites pass locally

Does this PR entail a change in behavior?

  • Yes — described under Summary above
  • No

Astro-Han
Astro-Han reviewed Aug 26, 2026
View reviewed changes

@Astro-Han Astro-Han left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I reviewed this head and found blocking issues.

[P1] Batching caches unbounded raw output before redaction/caps

app-shell-session-events.ts:167-178 enqueues full tool_output_delta into pendingEvents; redaction and size caps only happen in applyToolOutputChunk() at frame time — burst can hold large unredacted data pre-paint → OOM risk.

Fix: redact and bound/coalesce at enqueue, not at paint.

[P2] Delayed frame resurrects cleared session state

Frame callback unconditionally projects pending events; cleanup doesn't drop batch and clear runs separately → delete session then frame creates placeholder turn/tool for deleted ID.

Fix: tie drop/cancel to teardown/delete with regression for delayed-frame-after-clear.

Hosted test/windows_recovery: SUCCESS, but code is NO-GO.

简体中文批量缓存与清理竞争问题。

Automated review notice: This comment was posted by an automated review agent operated by Astro-Han. It is not an independent human review and does not replace one.

@Colafornia

Colafornia commented Aug 26, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

Follow-up correction in e9e0bbc8f after reviewing the regression tests and runtime effects:

  • P1 — pushed back after tracing the complete path. Production tool_output_delta events come from createToolOutputDeltaEmitter, which redacts output and limits chunks to 8,192 characters. The Host protocol also bounds each frame, and applyToolOutputChunk keeps the Renderer state redacted and capped. The first response added another enqueue-time redaction pass and a synchronous flush at 200 pending events. Those changes duplicated hot-path work and let a flood bypass animation-frame batching, so they were removed.
  • P2 — retained and fixed at teardown. clearSessionRendererState drops that Session from the shared display batch before clearing UI state. The delayed frame then has no event that can recreate the deleted projection.

Verification:

  • npm --workspace @maka/desktop run build:main
  • node --test apps/desktop/dist/main/__tests__/streaming-handoff.test.js
  • Scoped Biome check for the three affected files
  • git diff --check

The remaining regressions cover the intended frame publication bound and the delayed-frame-after-clear race.

@Colafornia
Colafornia force-pushed the fix/desktop-tool-output-jank branch from e9e0bbc to e98bbf0 Compare August 26, 2026 17:44
@github-actions github-actions Bot added the effort/M Under 500 readable lines label Aug 27, 2026
@Colafornia
Colafornia force-pushed the fix/desktop-tool-output-jank branch from e98bbf0 to fcfa981 Compare August 28, 2026 07:01
Astro-Han
Astro-Han approved these changes Aug 28, 2026
View reviewed changes

@Astro-Han Astro-Han left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed at exact head fcfa981eb15d9127e94b486d2e240923d72e77c8. Approving with comments (P2 only).

The delayed-frame-after-clear race is fixed (dropDisplayEvents from clearSessionRendererState). Agreed that a synchronous flush every N events would fight this PR's animation-frame batching, so that path should not come back.

[P2] Putting tool_output_delta through scheduleDisplayEvent still leaves pendingEvents uncapped. Each producer chunk is already bounded, but the renderer list is not: a flood can sit until rAF (or the 100ms timeout) and then apply as one long paint. Please drop-oldest / coalesce in the pending structure (reuse the existing 200-chunk / 16KiB live-turn cap) without extra setState.

CI test on this SHA is green. Not blocking.

Comment thread apps/desktop/src/renderer/app-shell-session-events.ts
@Colafornia
perf(desktop): batch live tool output
24a025e 
Publish high-frequency tool output once per animation frame while preserving synchronous lifecycle ordering.\n\nBound pending output with the existing live-turn limits and discard queued events when a session is cleared.\n\nGenerated-by: Codex
@Colafornia
Colafornia force-pushed the fix/desktop-tool-output-jank branch from 0419549 to 24a025e Compare August 28, 2026 09:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@Astro-Han Astro-Han Astro-Han approved these changes

Assignees

No one assigned

Labels

effort/M Under 500 readable lines

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@Colafornia @Astro-Han